Use monorepo-diff-buildkite-plugin from the PATH if available. #77
Conversation
There was a problem hiding this comment.
I believe a better way to implement the same thing would be to:
- change the
test_modevariable to something more likedownload_if_not_exists - just check that the command exists (with
check_cmd) and fail if it doesn't
That way there is no need for additional variables or logic, defaults will be more secure or stable and there is a more explicit indication that something will be downloaded off the internet... even if that means a major release due to backwards-incompatible changes.
Finally, it would require the appropriate tests and documentation
| @@ -96,18 +96,25 @@ download_binary_and_run() { | |||
| fi | |||
|
|
|||
| local test_mode="${BUILDKITE_PLUGIN_MONOREPO_DIFF_BUILDKITE_PLUGIN_TEST_MODE:-false}" | |||
| local _command="./${_executable}" | |||
There was a problem hiding this comment.
using this default value hides the fact that the variable is then used incorrectly unless the value is modified to remove the starting ./
| if check_cmd monorepo-diff-buildkite-plugin; then | ||
| _command="$(command -v monorepo-diff-buildkite-plugin)" | ||
| else |
There was a problem hiding this comment.
I would add a way to still force a download
I want to make sure I understand properly. You're asking me add a new YAML parameter to the plugin, something like Then I will modify the script so that the script only does the download of the binary when that parameter is true, otherwise it will use
It appears that the tests over this functionality were intentionally removed in this PR. As far as I can tell, the current tests only test that the plugin configuration is loaded properly, not actually executing the bash script (although maybe I'm misunderstanding). You're just asking me to include tests to cover the new |
Correct. Most of the logic for that is already behind the
That is correct, but tests were secondary to the documentation of the option and the plugin behaviour. |
|
It should also make sure that the correct version is installed. |
Resolves #76
This PR attempts a relatively simple fix, by checking if the plugin is already on the PATH when executing the command, and choosing the pre-existing copy if so.
Users who are concerned about re-downloading the binary can then modify their Buildkite Agent Workers to have a pre-cached binary that they have checksum'd and validated as they please.